Skip to content

Conversation

@roomote
Copy link
Collaborator

@roomote roomote commented Jun 19, 2025

Summary

This PR fixes the inconsistent cancellation error messages reported in issue #4903. Previously, when users cancelled during the thinking phase of reasoning models (like Claude 4.0 Sonnet), they would see a misleading red 'API Streaming Failed' error instead of the expected grey 'API Request Cancelled' message.

Changes Made

  • Modified to properly detect user-initiated cancellations
  • Added logic to check if is true when handling streaming errors
  • When a user cancels (abort = true), the cancellation is now consistently treated as 'user_cancelled' regardless of whether it occurs during thinking or streaming phases
  • When it's an actual streaming error (abort = false), it's still treated as 'streaming_failed'

Testing

  • Existing tests continue to pass
  • The fix ensures consistent UX for user-initiated cancellations across all phases of model execution

Related Issues

Fixes #4903
Related to #4039

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

…treaming phases

- Modified Task.ts to check if cancellation was user-initiated (this.abort = true)
- When user cancels during thinking phase, now shows 'API Request Cancelled' instead of 'API Streaming Failed'
- Ensures consistent UX for user-initiated cancellations regardless of when they occur
@roomote roomote requested review from cte, jr and mrubens as code owners June 19, 2025 21:38
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jun 19, 2025
@ellipsis-dev
Copy link
Contributor

ellipsis-dev bot commented Jun 19, 2025

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at [email protected]


Generated with ❤️ by ellipsis.dev

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 19, 2025
Copy link
Collaborator

@hannesrudolph hannesrudolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I've left a couple of comments on the PR:

  1. An unnecessary log file seems to have been included in the commit.
  2. A question about test coverage for the cancellation logic change.

Please take a look when you have a moment.

? undefined
: (error.message ?? JSON.stringify(serializeError(error), null, 2))

await abortStream(cancelReason, streamingFailedMessage)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to correctly handle the inconsistent cancellation message. I'm curious if there are any existing tests that cover this scenario, or if we could add a new one to ensure this behavior is protected against regressions?

@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 19, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jun 20, 2025
@daniel-lxs
Copy link
Member

This fix seems to solve the issue:

image

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 22, 2025
@mrubens mrubens merged commit fb711a0 into main Jun 30, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 30, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 30, 2025
@mrubens mrubens deleted the fix-4903 branch June 30, 2025 14:49
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 30, 2025
hannesrudolph pushed a commit that referenced this pull request Jul 3, 2025
utarn pushed a commit to modelharbor/ModelHarbor-Agent that referenced this pull request Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Bug: Inconsistent cancellation error messages - "API Streaming Failed" during thinking vs "API Request Cancelled" during streaming

5 participants